[Nexthop] don't crash sw_agent in handlePendingUpdates when HwSwitch connections are lost - #1544
Open
rtl-nexthop wants to merge 4 commits into
Open
Conversation
Signed-off-by: Vishrant Vasavada <vvasavada@nexthop.ai>
…s are lost When fboss_hw_agent restarts shortly after fboss_sw_agent starts (while the fresh sw agent is still applying its initial updates), the pending update fails: the oper delta stream disconnects or the ack times out and MultiSwitchHwSwitchHandler cancels the update. HwSwitchConnectionStatusTable already creates cold boot markers and schedules a graceful shutdown at that point, but the shutdown runs on another event base, so the update thread can observe applied != desired before the EXITING run state is set. handlePendingUpdates() then hits "Failed to apply update to HW and the update is not marked for HW failure protection" and SIGABRTs. Treat "all HwSwitch connections lost" like the existing isExiting() case: log an error, bump a new hwswitch_disconnected_update_drop counter so the drop is alertable, (re)request the graceful shutdown, and let the queued updates complete instead of crashing. State is resynced anyway via the cold boot forced on restart. Genuine HW programming failures with a live connection still FATAL, HW-failure-protected updates still throw FbossHwUpdateError to the caller, and monolithic mode is unaffected (hasActiveHwSwitchConnections() is always true there). Signed-off-by: rtl-nexthop <rtl@nexthop.ai>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fboss_sw_agentcrashes with SIGABRT when the hw agent is restarted ~1–2safter the sw agent starts — i.e. while a fresh sw agent (coldboot) is still
applying its initial pending updates. Under config load this reproduces
every time. The same fatal also fingerprints on mid-test hw agent restarts.
Crashing stack:
SwSwitch::handlePendingUpdates→LOG(FATAL) "Failed to apply update to HW and the update is not marked for HW failure protection".Root cause
A shutdown/update race in split-agent (multi_switch) mode:
either times out waiting for its ack or hits the DISCONNECTED sync state,
and
MultiSwitchHwSwitchHandler::stateChangedreturnsHWSWITCH_STATE_UPDATE_CANCELLED(nothing applied).HwSwitchConnectionStatusTable::disconnected()sees the last connectiondrop, creates cold boot markers, and calls
requestGracefulShutdown()—but that only schedules teardown on another event base; the EXITING run
state is set later, inside
stop().isExiting()(still false — the race) andhwFailureProtected()(falsefor initial config/route updates), so it falls into the FATAL.
The CANCELLED status — "connection lost, shutdown imminent", deliberately
distinct from FAILED — is dropped in
MultiHwSwitchHandler::stateChanged,so
SwSwitchcan't tell a connection loss from a real programming failure.Fix
Treat "all HwSwitch connections lost" the same as "already exiting" at the
FATAL site:
HwSwitchConnectionStatusTable::hasActiveConnections()— new accessor.MultiHwSwitchHandler::hasActiveHwSwitchConnections()— always true inmonolithic mode (mirrors
isHwSwitchConnected), else consults the table.SwSwitch::handlePendingUpdates: if the update failed and no HwSwitchconnections remain, log an error, increment a new
hwswitch_disconnected_update_dropcounter so the drop is alertable(the sole evidence was otherwise one ERR line per dropped update), request
the graceful shutdown, and fall through like the
isExiting()branch soblocking callers complete.
The
requestGracefulShutdown()here is belt-and-braces: every path thatempties the connection table already requests one (and
call_oncecollapsesthe requests), but the drop path must not depend on that invariant — dropping
updates with no pending teardown would leave a zombie agent acking updates
that never touched hardware. A comment at the call site records this so it
doesn't read as dead code.
The connection-table check is a proxy for the dropped CANCELLED status. It is
correct because both cancellation paths erase the table entry before
stateChangedreturns to the update thread: the stream-disconnect path vianotifyHwSwitchDisconnected, and the ack-timeout path via thedisconnected()call insideMultiSwitchHwSwitchHandler::waitForOperSyncAck.A TODO at the site documents the exact alternative — propagating the aggregate
update status out of
MultiHwSwitchHandler::stateChanged— which would notdepend on that ordering and would also cover partial cancellation in
multi-HwSwitch topologies (one of several HwSwitches cancelled), a pre-existing
gap not addressed here.
Behavior is unchanged for genuine HW programming failures with a live
connection (still FATAL), HW-failure-protected updates (still throw
FbossHwUpdateErrorto the caller), and monolithic mode.Test Plan
New unit test
SwSwitchHandlerTest.updateFailureWithNoActiveHwConnectionsDoesNotCrashin
fboss/agent/test/SwitchHandlerTest.cppcovers the dropped update, thecounter bump, and the graceful-shutdown request. This change also adds a
switch_handler_testcmake target soSwitchHandlerTest.cppis built and runby the OSS cmake build, which it was not before.
Verified with the cmake build on this exact tree: builds clean, and the unit
test suite passes 1947/1947 including the new test.
The agent-coldboot system test that previously crashed reliably passes with
this change.